Skip to content

chore: reorganize tsconfig files for clarity - #257

Merged
ulissesferreira merged 1 commit into
mainfrom
chore/rename-and-reorganize-tsconfig-files
Sep 3, 2026
Merged

chore: reorganize tsconfig files for clarity#257
ulissesferreira merged 1 commit into
mainfrom
chore/rename-and-reorganize-tsconfig-files

Conversation

@ulissesferreira

@ulissesferreira ulissesferreira commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Explanation

One more clarity improving PR for the tsconfig files on our repository so anyone can understand what is happening under the hood and keeping as many similarities as possible with core. The hierarchy of the config files is explained below but I believe that with their names and comments inside it should be self explanatory. Please raise if this is not extremely intuitive.

tsconfig.base.json                              - Configurations shared by all `tsconfig` files
│
├── tsconfig.json                               - Repository root configuration
├── tsconfig.scripts.json                       - Used for the `scripts/` directory
└── tsconfig.packages.json                      - Shared by all `packages/`, whether Snaps or not
    │
    ├── 👻 tsconfig.packages.libs.json     - Not needed because it's the same thing as
    │                                             `tsconfig.packages.json`
    ├── tsconfig.packages.libs.build.json  - Needed changed for the emission of
    │                                             declarations
    │
    ├── tsconfig.packages.snaps.json            - Snap specific configurations
    └── 👻 tsconfig.packages.snaps.build.json   - Not needed because mm-snap handles
                                                    everything automatically

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

@ulissesferreira
ulissesferreira requested review from a team as code owners September 2, 2026 16:16
@ulissesferreira
ulissesferreira force-pushed the chore/rename-and-reorganize-tsconfig-files branch 3 times, most recently from b58adfc to a754be9 Compare September 2, 2026 16:39
taran-a
taran-a previously approved these changes Sep 2, 2026

@taran-a taran-a left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. GG

Comment thread scripts/create-package/package-template/tsconfig.build.json Outdated
Comment thread scripts/create-package/package-template/tsconfig.json Outdated
Comment thread tsconfig.packages.libs.build.json
Comment thread tsconfig.packages.non-snaps.build.json Outdated
Comment thread AGENTS.md Outdated
Comment on lines +8 to +12
/**
* Unlike Snap's which include all TypeScript, TSX and JSON files, here we want
* to mirror what gets built.
*/
"include": ["./src"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about why was this change needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, removing the unused types entry and the exclude is inherited from the shared tsconfig's above

Comment on lines +6 to +10
/**
* Unlike Snap's which include all TypeScript, TSX and JSON files, here we want
* to mirror what gets built.
*/
"include": ["./src"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about why was this change needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2026-09-03 at 10 42 23

So what that line is doing is making sure that TypeScript is only applied to the actual src files instead of also including things under the whole package folder. Not including it would add more than actually needs to be checked.

@ulissesferreira ulissesferreira Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I have realized one thing. Instead of specifying what we want included, I think it would be best to specify what we exclude because if in the future we add a scripts folder inside here or if we change some config files to use TS instead of JS, we want to already be typechecking those.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last comment does not apply to the build though. For the build we want to specify exactly what is being included and therefore being include-first is the best approach to avoid shipping bloat in the package.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the precious line, which add a ../../types was pointing to something that didn't exist 😅

@ulissesferreira
ulissesferreira force-pushed the chore/rename-and-reorganize-tsconfig-files branch 4 times, most recently from 748bb61 to 17ebfdd Compare September 3, 2026 09:10
@taran-a

taran-a commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Hi @ulissesferreira, spotted this comment in the core PR MetaMask/core#10019

it is related to the target ES2023, which is not supported in ReactNative, maybe it is irrelevant for snaps, but worth checking

@ulissesferreira
ulissesferreira force-pushed the chore/rename-and-reorganize-tsconfig-files branch from 17ebfdd to 427d4b6 Compare September 3, 2026 09:17
@ulissesferreira

Copy link
Copy Markdown
Contributor Author

Hi @ulissesferreira, spotted this comment in the core PR MetaMask/core#10019

it is related to the target ES2023, which is not supported in ReactNative, maybe it is irrelevant for snaps, but worth checking

Thank you for the thoughtfulness! As part of this work I am asking the Snaps team to have a look and tell us if they like the configurations for Snaps. For our library package I guess the same thing kind of applies. I was expecting whatever bundler used by the clients to handle that actually, kinda surprised Core has to be the repository that implements that.

@ulissesferreira
ulissesferreira force-pushed the chore/rename-and-reorganize-tsconfig-files branch 2 times, most recently from effb589 to 8a31ec9 Compare September 3, 2026 10:27
Comment thread tsconfig.json
},
"files": [],
"include": ["./docs", "./tests", "./scripts"],
"exclude": ["**/node_modules"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a tsc default, unnecessary.

"locales/*.json",
"snap.manifest.json",
"src/**/*.json"
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify...

@ulissesferreira
ulissesferreira force-pushed the chore/rename-and-reorganize-tsconfig-files branch from 8a31ec9 to 42d9b09 Compare September 3, 2026 10:39
@ulissesferreira
ulissesferreira added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit b8e5aba Sep 3, 2026
54 checks passed
@ulissesferreira
ulissesferreira deleted the chore/rename-and-reorganize-tsconfig-files branch September 3, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants